feat(keeper): integrate filesystem backend with keeper trait - #583
feat(keeper): integrate filesystem backend with keeper trait#583aldy505 wants to merge 4 commits into
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| keeper: KeeperConfig { | ||
| backend: KeeperBackend::Sqlite, | ||
| connection_url: "sqlite:///opt/objectstore/keeper.db".into(), | ||
| }, |
There was a problem hiding this comment.
Broken default keeper database path
High Severity
The default connection_url points at /opt/objectstore/keeper.db, a path that does not exist elsewhere in this project. Local runs, Docker (/data), README env setup, and e2e tests all rely on this default when keeper is unset, so filesystem backend startup will fail creating the SQLite database.
Reviewed by Cursor Bugbot for commit 499ea21. Configure here.
| _ => Err(Error::generic(format!("unknown backend {}", s))), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Keeper backend config case mismatch
Medium Severity
KeeperBackend deserializes as Sqlite, but the filesystem config docs and FromStr accept sqlite. Config written as documented (or via env) will fail to parse. Other config enums in this repo use rename_all = "lowercase", and the new FromStr impl is never wired into serde.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 499ea21. Configure here.
| { | ||
| objectstore_log::debug!("Object not found"); | ||
| } | ||
| self.keeper.remove(id).await?; |
There was a problem hiding this comment.
Keeper removed on failed deletes
Medium Severity
delete_object always calls keeper.remove before returning the filesystem delete result. If remove_file fails for a reason other than not found, the keeper entry is dropped while the object file remains, so TTL/TTI tracking is permanently lost for that object.
Reviewed by Cursor Bugbot for commit 499ea21. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| self.keeper.keep(id, metadata.expiration_policy).await?; | ||
|
|
There was a problem hiding this comment.
Bug: Overwriting an object with a TTL/TTI policy fails due to a database UNIQUE constraint violation, causing data inconsistency despite the file being successfully updated.
Severity: HIGH
Suggested Fix
Modify the keeper.keep() method to handle cases where an object ID already exists. Instead of a simple INSERT, use an INSERT OR REPLACE or an equivalent UPSERT statement. This will ensure that overwriting an object also correctly updates its expiration policy in the keeper database without causing a constraint violation.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: objectstore-service/src/backend/local_fs.rs#L136-L137
Potential issue: When an object with a non-manual expiration policy (e.g., TTL or TTI)
is overwritten, the file is successfully updated on the filesystem, but the overall
operation fails. This occurs because the `keeper.keep()` method attempts to `INSERT` a
new record for the object's expiration into the SQLite database. Since a record for that
`object_id` already exists, this action violates the `PRIMARY KEY` constraint. The
resulting SQL error is propagated, causing the `put_object` request to fail, leading to
an inconsistency between the file storage and the expiration keeper database.
Also affects:
objectstore-service/src/backend/local_fs.rs:457~458
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit eba4ba5. Configure here.
| file.sync_data().await?; | ||
| drop(file); | ||
|
|
||
| self.keeper.keep(id, metadata.expiration_policy).await?; |
There was a problem hiding this comment.
Overwrite breaks keeper retention tracking
High Severity
put_object and complete_multipart always call keeper.keep, but keep only inserts and rejects duplicates. Overwriting an object with a client-supplied key fails after the file is already replaced, or silently leaves a stale retention row when the new policy is Manual.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit eba4ba5. Configure here.
| { | ||
| objectstore_log::debug!("Object not found"); | ||
| } | ||
| self.keeper.remove(id).await?; | ||
| Ok(result?) |
There was a problem hiding this comment.
Bug: In delete_object, a failure in self.keeper.remove(id) can mask the result of the filesystem deletion, potentially causing the caller to believe an object exists when it has been deleted.
Severity: HIGH
Suggested Fix
Handle the result of the filesystem deletion before attempting the keeper database operation. For example, if the file is not found, return the NotFound error immediately. If the file is deleted successfully, proceed with the keeper operation but ensure its potential failure does not obscure the successful deletion.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: objectstore-service/src/backend/local_fs.rs#L195-L199
Potential issue: In the `delete_object` function, the result of the filesystem deletion
is stored but not immediately returned. An unconditional call to
`self.keeper.remove(id).await?` follows. If this database operation fails (e.g., due to
an I/O error or the database being unavailable), the `?` operator will propagate the
database error, and the original result from the filesystem operation will be discarded.
This can lead to an inconsistent state where a file is successfully deleted from the
filesystem, but the caller receives a database error, leading it to believe the object
still exists.
1 participant
No description provided.